Skip to content

feat(github): show every target's plan, not just the reviewed one - #1425

Open
aparajon wants to merge 59 commits into
armand/multi-target-plan-groupsfrom
armand/multi-target-plan-group-bodies
Open

aparajon wants to merge 59 commits into
armand/multi-target-plan-groupsfrom
armand/multi-target-plan-group-bodies

Conversation

@aparajon

@aparajon aparajon commented Sep 17, 2026 •

Copy link
Copy Markdown
Collaborator

Targets under a targets: list are planned each and converge on their own, so
they are free to hold different schemas and usually do. The plan comment already
named that divergence, but it still rendered only the reviewed plan's DDL. An
operator was being asked to authorize work the comment never showed them.

Each distinct plan now renders under the members that would run it. The reviewed
plan stays open, the rest collapse behind a line saying the apply runs them too,
and a group already at the desired schema is named rather than hidden. The
summary line counts the rollout rather than the reviewed plan alone.

A primary already at the desired schema also no longer short-circuits the whole
comment to "No schema changes detected" while its siblings still have work.

In the example below primary/testapp_1 is the reviewed target and alters
users, primary/testapp_2 plans the same change, and primary/testapp_3 plans
that alter plus an index.

Before                                          After

+------------------------------------------+    +------------------------------------------+
| Planned separately for 3 targets         |    | Planned separately for 3 targets         |
| 2 distinct plans. Each applies its own   |    | 2 distinct plans. Each applies its own   |
+------------------------------------------+    +------------------------------------------+
| ALTER users ADD email                    |    | [open] testapp_1 (primary), testapp_2    |
|                                          |    |        - 1 DDL statement                 |
|   x testapp_3's second statement is      |    |        ALTER users ADD email             |
|     nowhere on the comment               |    +------------------------------------------+
|                                          |    | [+] testapp_3 - 2 DDL statements         |
|                                          |    +------------------------------------------+
|                                          |    | ! Applying runs each target's own plan,  |
+------------------------------------------+    |   including the collapsed one            |
| Plan: 1 table to alter                   |    +------------------------------------------+
|   x counts the primary alone             |    | Plan: 2 distinct plans on 3 targets      |
+------------------------------------------+    +------------------------------------------+

This upholds RV-3: the consent an operator gives is specific to what will
run, and what will run is every target's plan, not one member's.

Targets converging: before

Schema Change Plan — Production

Database: testapp | Type: MySQL | Schema Name: testapp

Requested by @jackjackbits at 2026-01-01 00:00:00 UTC · planned from abcdef1

✅ Planned separately for all 3 targets (primary/testapp_1, primary/testapp_2, primary/testapp_3) — 2 need this change, 1 is already at this schema.

ALTER TABLE `users` ADD COLUMN `email` varchar(255);

📋 Plan: 1 table to alter


▶️ To apply all schema changes from this PR, comment:

schemabot apply -e production
Targets converging: after

Schema Change Plan — Production

Database: testapp | Type: MySQL | Schema Name: testapp

Requested by @jackjackbits at 2026-01-01 00:00:00 UTC · planned from abcdef1

✅ Planned separately for all 3 targets (primary/testapp_1, primary/testapp_2, primary/testapp_3) — 2 need this change, 1 is already at this schema.

primary/testapp_1 (primary), primary/testapp_3 — 1 DDL statement

ALTER TABLE `users` ADD COLUMN `email` varchar(255);

primary/testapp_2 — already at this schema, nothing to apply.

📋 Plan: 1 DDL statement on 2 of 3 targets


▶️ To apply all schema changes from this PR, comment:

schemabot apply -e production
Targets diverging: before

Schema Change Plan — Production

Database: testapp | Type: MySQL | Schema Name: testapp

Requested by @jackjackbits at 2026-01-01 00:00:00 UTC · planned from abcdef1

✅ Planned separately for all 3 targets (primary/testapp_1, primary/testapp_2, primary/testapp_3) — 2 distinct plans. Each target applies its own.

ALTER TABLE `users` ADD COLUMN `email` varchar(255);

📋 Plan: 1 table to alter


▶️ To apply all schema changes from this PR, comment:

schemabot apply -e production
Targets diverging: after

Schema Change Plan — Production

Database: testapp | Type: MySQL | Schema Name: testapp

Requested by @jackjackbits at 2026-01-01 00:00:00 UTC · planned from abcdef1

✅ Planned separately for all 3 targets (primary/testapp_1, primary/testapp_2, primary/testapp_3) — 2 distinct plans. Each target applies its own.

`primary/testapp_1` (primary), `primary/testapp_2` — 1 DDL statement
ALTER TABLE `users` ADD COLUMN `email` varchar(255);
`primary/testapp_3` — 2 DDL statements
ALTER TABLE `users` ADD COLUMN `email` varchar(255);

ALTER TABLE `users` ADD INDEX `idx_email`(`email`);

⚠️ Applying runs each target's own plan, including the ones collapsed above.

📋 Plan: 2 distinct plans on 3 targets


▶️ To apply all schema changes from this PR, comment:

schemabot apply -e production

Opened by Claude (Claude Opus 5).

aparajon and others added 30 commits September 10, 2026 18:16
The review-time drift rollup identified each member of a multi-deployment
rollout by its deployment name alone. That holds only while a deployment
addresses exactly one target, so the positional producer/rollup contract
could not distinguish two members that share a deployment.

Key the contract on the (deployment, target) pair via a MemberID helper on
routing.ExecutionTarget, thread the pair through the producer's primary
invariant, and carry the plan's origin target alongside its origin
deployment so the baseline is verified against the member it was created
for. Behavior is unchanged for every configured shape today, where each
deployment addresses one target.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An apply whose members are planned together shares one plan, and that is
still the common case. When each member is planned against its own live
schema, though, there is no single plan for the apply to point at.

Add a nullable apply_operations.plan_id so a member can name the plan it
executes, with PlanIDForOperation resolving to the parent apply's plan
when the member has none. An operation with no plan on either row is not
executable and now errors rather than resolving to a row ID no plan has.

TargetOperationKey names the operation key for one target's work when a
single apply addresses several targets, composing with the shard-scoped
key so a sharded target still gets one key per shard.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The error identified the operation by its internal numeric row ID, which
is not a triage handle an operator can look up. Name the operation by the
identifiers that route it instead.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A database that lives on more than one target has no single target to
name. Add a targets list, on an environment and on a deployments-map
entry alike, and resolve it to one rollout member per target — the
deployment and target pair a member is identified by.

Validation and routing share one resolver for the list, so a config that
validates resolves to exactly the member list validation checked. An
entry names either a target or a targets list, never both; a list may not
be empty, hold an empty entry, or repeat a target, since one deployment
cannot address the same target twice.

Members resolve deployments outermost, so a rollout finishes one
deployment's targets before moving to the next.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Addressing several targets from one database entry has to be represented
on every operator-facing surface — the plan comment, progress, the
terminal summary — and that presentation is built per engine. Enable the
targets spelling per engine as that work lands rather than assuming it
everywhere: a targets list on a vitess, strata, or postgres database now
fails config validation at startup.

The gate is on the spelling, not the member count. A deployments map
whose entries each name a single target is not multi-target however many
entries it has, and stays available on every engine.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An entry that spells out both `target` and `targets: []` was reported as
an empty list rather than as the conflict it is. Both spellings being
present is what conflicts, so gate on the list being configured at all.
Document that an environment-level targets list excludes a deployments
map the same way an environment-level target does.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An environment whose targets each hold their own schema has no single
live schema, so pulling only the primary presented one target's schema
as the environment's. A pull now fans out across every target and reports
how each one differs from the primary.

The response body is still the primary's schema, which is what a caller
materializes. Alongside it, each other target reports its table count and
the tables the two do not agree on: held by both with different DDL, held
only by the primary, or held only by that target. Tables are compared by
the dialect parser's canonical form, so formatting never reads as a
schema difference.

Deployments that are expected to hold the same schema are neither pulled
nor compared. A difference between them is drift for the review-time
rollup to block on, and pulling them would cost a round trip each to
learn what the configuration already asserts.

A target that cannot be pulled, or a table whose DDL cannot be compared,
fails the pull. Reporting no divergence for a target that was never
compared would describe the environment as converged on the strength of a
comparison that did not happen.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ed independently

An environment whose members are planned against their own live schemas
has no single plan that covers them, so each non-primary member's plan is
now stored as a plan row of its own and its identifier recorded on the
member's rollup entry. The primary is deliberately left without one: its
plan is the reviewed plan the apply is created from.

Plan-row construction moves into one helper shared by the reviewed
primary plan and a member plan, so the two are stored identically and are
indistinguishable downstream. An identifier that already exists still
means "the same plan, re-stored", and the existing row is now reloaded so
its ID is returned rather than lost.

A member whose plan cannot be stored is reclassified as errored and
blocks the review: an apply dispatches each member against its stored
plan, so a member with no stored plan has nothing to run.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An apply resolves each rollout member to the plan its work is built from
before building operations. Members of an environment whose members hold
the same schema all carry the apply's own plan. A member that was planned
against its own live schema carries its own plan, looked up by member id
and bound to the head SHA the apply's plan was created for, so plans from
an earlier push of the same pull request are never picked up.

A member with no plan for that review round fails apply creation. The
apply's plan describes a different target's schema, so substituting it
would run DDL that target was never planned for.

Operations are grouped by member, and a member is its deployment and
target together, so two targets of one deployment each get their own
operation for a given (namespace, shard, table) rather than sharing one.
An operation names a plan of its own only when it runs a different plan
than its apply.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A pull of an environment whose targets each hold their own schema listed
only the targets compared against the primary, leaving the primary itself
implicit in the response body. A caller reconciling the environment against
its own shard inventory then reads a four-target environment as three, and
can only recover the missing member by knowing which target the schema in
`namespaces` came from.

Every target the environment addresses is now named on `targets`, in
configuration order, with the primary carrying `"primary": true` and no
comparison of its own. The primary's schema is already in hand from the pull
that produced the response body, so naming it costs no extra round trip.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ocking on drift

An environment that spells its routing as a targets list holds several
distinct databases under one deployment, so its members are not expected
to hold the same schema and a difference between them is not drift.
Plan such an environment's members independently: classify each member
that produced a usable plan as Planned rather than comparing it to the
reviewed primary.

The choice is fail-closed. An environment plans independently only when
a targets list appears, at the environment level or inside a deployments
entry; every other shape, including every config that predates the
targets spelling, stays mirrored and keeps blocking on drift.

Independent planning still blocks on a member that could not be planned:
a producer error, or change content that will not parse under the
member's own grammar.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A plan whose identifier is already stored is that same plan re-stored, and
the store's job is done. Storing a member plan needs no row ID today —
the member's rollup entry carries the plan identifier — so plan storage
stays a write that either succeeds or reports why, with no read-back that
could fail a plan the store already holds.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The trusted control-plane enqueue path holds no `Databases` config, so
resolving the member planning contract there fails and apply creation
with it. A single member is the plan's own primary: it runs the apply's
plan under either contract, and no sibling's plan could be substituted
for it, so the contract lookup is unnecessary work at that point.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The schema intelligence guide documents the pull endpoint but not what a
pull of a multi-target environment returns. Describe the `targets` array,
what the primary entry means, the three `difference` values, and that an
empty `diverged_tables` is a positive statement that two targets agree
rather than a comparison that was skipped.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A rollout member is identified by its deployment and target together, but
the deployment alone is what the plan comment, check summary, apply comment
and CLI progress have been showing. Once one deployment can address several
targets, two members render under the same name, and a label that points at
another member ("waiting for X", "halted by X") no longer identifies one.

routing.DisplayNames is the single naming rule every surface now uses: a
member is named by its deployment alone unless that deployment addresses
more than one distinct target in the rollout, in which case every member of
that deployment is named deployment/target. Keying on distinct targets is
what keeps a keyed or sharded apply — several operations of one deployment
against the same target — named by the deployment, where the extra half
would be noise that still did not tell the operations apart.

presentation.Derive resolves each member's name once and hands it to every
consumer, including the labels that reference a sibling and the suggested
next action. The apply comment's per-member detail bodies move from a
name-keyed map to a slice paired positionally with the model, since a map
collapses two members of one deployment onto one body.

The independent-planning contract also reaches the wording: an errored
member reads "could not plan" where targets hold their own schemas and
"could not verify" where they are expected to mirror each other.
Table progress rows are attributed to a deployment, which was enough while
a deployment addressed one target. When it addresses several, each member
runs its own copy of the same tables, and a section scoped to the
deployment lists both members' copies under each of them.

Progress now carries the target alongside the deployment, from the stored
apply_operation through the API response to the CLI, and every consumer
selects on both halves of the routing pair. The external apply ID a member
inherits from a sibling that has already dispatched is gated the same way:
a keyed apply's operations share one data-plane apply and may inherit it,
while a deployment addressing several targets runs a separate apply per
target and inherits nothing.
An operation key names its target only when that target's deployment
addresses more than one, so a reader meets both shapes. CutTargetPrefix
matches the prefix against the operation's own target and reports whether
it cut one, leaving the caller to parse unqualified first so a key that
already reads whole is never mistaken for a qualified one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…miter

A deployment addressing several targets names each one in its members'
operation keys. A target carrying the delimiter would write a key no
reader can split back into the target it came from, and once such a key
is written the ambiguity is in the data. Config validation is the last
point at which it is still recoverable, so the name is refused there.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
MemberPlanningFor re-derived the targets-list predicate instead of asking
the helper that is meant to own it. A routing spelling added to the helper
would then select independent planning in validation and routing but
mirrored planning here, which is the one disagreement this contract cannot
survive.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A commit can be planned more than once — a re-plan, or two deliveries
racing — and each round stores its own plan per member with the same
route and the same head SHA. Nothing durable said which round a member
plan belonged to, so an apply created from one round's reviewed plan
could pair its members with another round's plans and dispatch DDL the
operator never saw.

Member plans now carry the reviewed plan's identifier, and a member plan
that cannot be attributed to a round is not stored at all. The member
diff also carries the request contract the primary's does: a namespace
the caller withheld is excluded from the desired state, not absent from
it, and the grouping choice is always stated. The execution-mode
vocabulary moves to the shared plan writer, so it is enforced once for
every plan row whichever RPC produced it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… named

An operation key is unique per deployment, so it identifies one operation
only when the deployment addresses one target. Where a deployment addresses
several, two members of it would otherwise store the same key for the same
table and collide on the target-blind unique index.

Keys are now qualified with the member's target in exactly that case, by the
same predicate that decides when a member is worth naming after its target.
A deployment with one target keeps the key shape every reader already parses.

A member whose own plan found nothing to change is recorded as completed on
creation, so the apply covers every member it addressed rather than leaving a
pending operation no driver can finish.

Member plans are now resolved by the review round they were stamped with
rather than by head SHA and first-row-wins. The round takes precedence over
current config: a round that planned its members independently is applied that
way even if the environment has since been respelled as mirrored. Config
decides only the case the round is silent about, and fails closed there.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A rollout member planned against its own live schema names its plan on its
operation row. Dispatch loaded the apply's plan regardless, so a member's work
was sent with the primary's schema files and recorded against the primary's
plan identifier — one target's DDL described by another target's plan.

The scope now resolves the plan it runs: an operation's own when it has one,
its apply's otherwise, and an error when neither row names a plan rather than a
zero a caller could mistake for a valid plan.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…er-naming

# Conflicts:
#	pkg/routing/resolver.go
#	pkg/routing/resolver_test.go
A rollout member whose own plan found nothing to change has no task to
carry. Apply creation records it already completed, so the apply covers
every member it addressed instead of silently omitting one — but the
grouped-insert guard refused any task-less work operation and rejected
the whole apply.

The guard's reason is that an operation-scoped drive must never claim
work it cannot find, which is a statement about operations still to be
driven. It now refuses a task-less work operation only while the
operation is non-terminal; a group_finalizer rebuilds its work from the
plan and a terminal operation is never claimed, so neither can strand a
driver by arriving empty.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
aparajon and others added 16 commits September 14, 2026 17:01
The drive resolves its plan from the operation it is driving, so the
operation is passed at every call rather than through a second entry
point that only ever passed none.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A work operation with no tasks is valid only when its plan carries VSchema
work, so resolving the plan is part of deciding that. An operation naming no
plan, under an apply naming none either, has nothing that could make it valid
and is the same invalid or stale claim: it must terminalize on the fail-closed
signal the operator matches, not on a distinct resolution error that leaves the
claim to be re-leased forever. The resolution failure rides along as context so
the cause stays triageable.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A multi-target environment plans every member against its own live schema, so
one review round produces N plans. The plan comment has to group the members
that would run the same work, and the only honest way to decide "same work" is
the comparison the drift rollup already performs -- comparing rendered DDL text
would split a group over a backtick.

ChangeSetFingerprint returns a stable key over the same canonicalized multiset
CompareChangeSets keys on, so two change sets share a key exactly when the
comparison reports them identical. That makes grouping by it sound rather than
a heuristic: members of one environment are planned by the same differ against
the same desired schema, so the only thing that can make two plans differ is
the live schema each was diffed against, and any such difference changes the
multiset. A table test pins fingerprint equality against the comparison's own
verdict across restyled, reordered, duplicated, and diverging sets, so the two
cannot drift apart without failing.

Three properties the key needs and the tests pin by construction. It sorts the
rendered records, because a multiset and a set are both unordered and the
engine's return order is not work. It counts changes rather than collecting
them, because running a change twice is not running it once. And it separates
the fields it joins with a byte no field can hold -- a namespace, table, or DDL
body is whatever a schema author wrote, so a concatenated key would let a
namespace ending where a shard name begins collide two members into one group.

It errors on exactly what the comparison errors on, so a member whose change
set cannot be canonicalized has no key at all and a caller has to refuse to
group it rather than file it with members it was never compared against.

Nothing calls it yet; the plan comment's grouping lands on top of it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A rollup entry recorded how a member classified but dropped the change
set that produced the classification, so a reader could say a member
diverged but not show what it would run. Every classified member now
carries its own change set and a fingerprint keying that change set by
the work it performs: members share the key exactly when the comparison
reports their plans identical, which lets a reader group members by plan
without comparing every pair.

A member that could not be planned carries neither. It has no plan to
describe, and keying it would group it with members it was never
compared against.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An environment whose members are distinct targets plans each one against
its own live schema, so the members are free to run different work. The
plan comment said only that: "each target holds its own schema, so their
plans are not expected to match". True of the contract, and silent about
the round in front of the reviewer. Targets free to differ usually do
not, and a fleet converging over several PRs — some targets changed, the
rest already there — was invisible.

The members are now grouped by the plan each would run, and the comment
states the result: every target needs the same change, or how many of
them are already at this schema, or how many distinct plans the apply
would run. Members group on the plan fingerprint, so they share a group
exactly when their plans are the same work. Each group carries the plan
its own members would run, in the shape the comment already renders the
reviewed plan in, so a later change can show it.

Grouping is confined to a clean rollup of independent members. A blocked
rollup still lists every member on its own, because the operator's next
step is the target that could not be planned. Mirrored members stay
ungrouped: a clean mirrored rollup has already proved they are one group,
and re-reporting that in the vocabulary of a fleet free to diverge would
read as an outcome rather than the requirement that let the check pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Targets under a `targets:` list are planned each and converge on their own,
so they are free to hold different schemas and usually do. The plan comment
named that divergence but still rendered only the reviewed plan's DDL, which
left an operator authorizing work the comment never showed them.

Each distinct plan now renders under the members that would run it. The
reviewed plan's block stays open, the rest collapse behind a consent line
saying the apply runs them too, and a member group already at the desired
schema is named rather than hidden. The summary line counts the rollout
instead of the reviewed plan alone.

A primary already at the desired schema no longer short-circuits the comment
to "no schema changes detected" when its siblings still have work to apply,
which upholds UX-3: the comment describes the apply an operator would
authorize, not one member of it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@aparajon
aparajon marked this pull request as ready for review September 17, 2026 20:51
@Kiran01bm

Copy link
Copy Markdown
Collaborator

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1425, f2ca09b.

Verdict: 4 findings — 2 blocking (unswept multi-env renderer, per-group DDL budget), 2 non-blocking.

Blocking

The multi-environment plan comment was not swept, so the misreport this PR fixes survives there. writeEnvironmentPlanSection still short-circuits on if totalChanges == 0 (the reviewed primary's count) and never calls writePlanGroups, while plan.go:466 hands it the same group-bearing rollup. Running head with staging changed and production's primary converged prints "2 need this change, 1 is already at this schema" immediately followed by "✅ No schema changes detected", dropping the two changing targets' DDL — and this is the auto-plan surface for any database with 2+ environments. Precedent is to sweep both renderers (writeDeploymentDrift was wired into both when drift landed).

The per-comment DDL budget is re-opened for every plan group, so the comment can exceed GitHub's hard 65536-char cap. writePlanGroups now calls writeKeyspaceChanges once per group and each call re-runs newDDLBlockBudget with remaining: maxCommentDDLLen, contradicting fence.go's "a budget shared across the blocks the comment renders"; vschemaDiffBudget resets the same way. Three ~25KB plans render ~75KB with no truncation marker, CreateIssueComment 422s, and plan_comment_retire.go:63 only logs the failure — the PR gets no plan comment while the check still records success and the stale comment is never retired.

Non-blocking

When the primary's group is empty, no group gets open, so every plan renders collapsed. Line 1271's docstring promises "A single plan is never collapsed", but the primary takes the g.Empty() continue before reaching the only place open is set, so the comment's sole DDL sits inside a closed <details> under "including the ones collapsed above". The operator authorizing the apply sees zero SQL without expanding; TestRenderPlanComment_ConvergedPrimaryStillShowsSiblingWork passes from inside the collapsed block.

Sibling groups' DDL is rendered on the consent surface with none of the disclosures that belong beside it. Line 1289 copies a member's own plan verbatim, but unsafe/blocked/lint/execution-mode data is derived from the primary response alone (plan.go:981), and a member's blocked verdict never errors the rollup. A sibling's DROP COLUMN therefore renders with no unsafe warning, no blocked section and no lint violation attached.

The one thing that could have broken, verified

The new && !groupsCarryWork(groups) on the no-changes short-circuit (plan.go:367) sits on the path every single-environment plan comment takes, so a wrong answer there would either drop real DDL or spam empty plan blocks on every PR. It cannot regress the ungrouped path: groups is nil there and slices.ContainsFunc over nil is false, so totalChanges == 0 still short-circuits exactly as before.

Verified correct

  • planGroups() gates on len(Plans) < 2, and Plans is only populated for a Clean+Independent rollup, so mirrored/blocked/unverified rollups keep pre-PR rendering.
  • planGroupWorkLabel(countChanges(g.Changes)) is a legal multi-value pass-through — (statements, vschemaNamespaces int) match.
  • The label's statements == 0 branch cannot print "0 vschema updates": Empty() is statements+vschema == 0 and empty groups continue earlier.
  • planGroupHeading's primary-is-first assumption holds: deploymentPlanGroups walks entries in rollout order with Primary at i == 0.
  • Member names pass through inlineCodeList before the " (primary)" suffix and <summary>, so hostile target names stay inside a code span.
  • writePlanGroupSummary re-renders writeIgnoredNamespaces and writeExemptTables, so those disclosures survive the group-summary substitution.
  • The converging preview's collapse=false is right — its only non-empty group is the primary, so no <details> is emitted and the TEMPLATES.md golden matches.

This review was generated by Claude Code (claude-opus-5).

@morgo morgo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Reviewed on Morgan's behalf, at f2ca09b1. Approving — the rendering rules are all pinned (seven of seven injections caught) and writeKeyspaceChanges really is reused rather than reimplemented, which is the thing I was watching for after #1424. Two findings, both about shapes the new branch structure opens up rather than about the main path.

1. When the primary's group is converged, nothing is expanded — every plan on the comment is collapsed

collapse is any(!Empty && !Primary) and open is set only on g.Primary. So in the exact shape this PR adds support for — a primary already at the desired schema while its siblings still have work — collapse is true, the primary's group takes the Empty() branch and never reaches the <details> code at all, and every group that does carry work renders with open = "".

Rendered, three targets, primary converged:

✅ **Planned separately for all 3 targets** — 2 distinct plans across the 2 targets that change; 1 is already at this schema.
**`primary/a` (primary)** — already at this schema, nothing to apply.
<details>
<details>
⚠️ Applying runs each target's own plan, including the ones collapsed above.

Two <details>, zero <details open>. The operator opens the PR and sees no DDL anywhere.

TestRenderPlanComment_ConvergedPrimaryStillShowsSiblingWork passes on this because it asserts Contains(out, "ALTER TABLE ..."), which is satisfied by text inside a collapsed block. So the case is tested for presence but not for visibility.

The doc comment states the rule as "the primary's group is open and the rest are collapsed… A single plan is never collapsed, since there is nothing to collapse it against" — which silently assumes the primary has work. Falling back to the first non-empty group when no group is both non-empty and primary is a one-line change and preserves the intent (the reader always has one plan in front of them).

2. The per-comment DDL and vschema budgets are now per-group, and the comment can exceed GitHub's limit

writeKeyspaceChanges constructs both budgets itself — newDDLBlockBudget(...) starts at maxCommentDDLLen (32768) and vschemaDiffBudget divides maxCommentVSchemaDiffLen (16384) — and its own comment says "The VSchema diff budget is per comment, not per keyspace". writePlanGroups now calls it once per group, so both caps are multiplied by the number of distinct plans.

Measured, with maxCommentDDLLen = 32768 and GitHubIssueCommentMaxChars = 65536:

2 groups, one large plan   ->  33,569 chars   (budget working)
4 groups, all large plans  -> 132,313 chars   (over GitHub's cap by 2x)

Nothing on the plan-comment path bounds the whole body today — GitHubIssueCommentMaxChars is consulted only in failure_logs.go and control_rejection_notice.go — so at this commit a four-plan rollout with substantial DDL renders a comment GitHub will reject.

I then read ahead: #1427 fixes this, with a whole-comment planCommentBudget and a withhold-and-re-render loop, so by the top of the stack the comment cannot overflow. Two things still worth saying. First, stacked PRs can land individually, and this one is the commit where the regression exists. Second, #1427's fix is a backstop on total size rather than a fix to the multiplication: each group still renders against a budget sized for a whole comment, so a rollout will withhold more group bodies than it would if the DDL budget were divided across the groups first. Passing a shared *ddlBlockBudget into writeKeyspaceChanges instead of letting it build its own would make the withholding a last resort rather than the normal outcome for a large multi-plan rollout.

Notes

  • writePlanGroupSummary has an unreachable-but-ungraceful plans == 0 arm. If every group were Empty() it would print "📋 Plan: 0 distinct plans on 3 targets". It cannot be reached: all-empty groups means groupsCarryWork is false, and the only way past the short-circuit is totalChanges != 0, which needs data.Changes to carry a statement the groups do not — and the paths that could produce that mismatch (an empty-DDL table change) fail canonicalDDLForDrift upstream and clear Clean before Plans is ever populated. Worth a line saying so, since the arm reads like a live default.
  • singleKeyspace heading suppression is now decided per group. writeKeyspaceChanges suppresses the #### Schema Name: heading when a plan has exactly one keyspace matching the database name; with scoped changes, one group can suppress it while a neighbour with two keyspaces shows it. Cosmetic, and arguably right per block, but the blocks are read side by side.
  • data.UnsafeChanges and data.BlockedChanges remain the primary's, rendered once below the groups. Correct as far as writeKeyspaceChanges goes — I checked, it reads only DatabaseType, IsMySQL, Database and Changes, all of which are rollout-wide except the one that is scoped. But the unsafe/blocked sections now sit under a set of blocks where only one of them is the plan those warnings were computed from. Not something to change in this PR; noting it because #1426 and beyond keep building on this layout.

What I checked rather than took on trust

  • Fault injection — seven for seven. Lowering the < 2 group threshold → OnePlanRendersAsTheReviewedPlan; dropping !groupsCarryWork from the short-circuit → ConvergedPrimaryStillShowsSiblingWork; forcing collapse false → DistinctPlansRenderUnderTheirMembers; never opening the primary → same; dropping the (primary) suffix → three tests; rendering the reviewed plan instead of the group's → two tests; disabling the plans == 1 summary arm → three tests.
  • scoped := data; scoped.Changes = g.Changes is safe. Read writeKeyspaceChanges end to end: the only per-plan field it touches is Changes. So the "same code that renders the reviewed plan" claim holds here, unlike the parallel builder in #1424.
  • planGroupHeading does not mutate the group. inlineCodeList allocates a fresh slice (make([]string, len(values))), so names[0] += " (primary)" cannot write back into g.Members.
  • Only one group can be open. Primary is set at group creation from i == 0 in #1424, so at most one group carries it, and writePlanGroups reads it directly.
  • planGroupWorkLabel(0, 0) would say "0 vschema updates", and is unreachable — the Empty() branch returns before the label is computed, and Empty() is exactly statements+vschema == 0.
  • The short-circuit change cannot regress the single-plan case. planGroups() returns nil below two groups, so groupsCarryWork(nil) is false and totalChanges == 0 short-circuits exactly as before for every non-multi-target comment.
  • Merge-base against pr1424 is an ordinary commit; +457/−4, the four deletions being the two call sites this branches. No test deletions. CI: 41 checks, no genuine failures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants